Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix JMXFetch issue when several instances on the same host #124

Merged
merged 4 commits into from
Feb 27, 2017

Conversation

zippolyte
Copy link
Contributor

@zippolyte zippolyte commented Feb 23, 2017

What this PR does

It fixes the issue occurring when refreshing shared connections by creating one connection per instance and thus removing shared connection.

Motivation

In the connection manager, it was possible to get a connection shared between different instances.
If at some point the connection gets broken or the App needs to refresh, it will force a new connection for every instance sharing this connection. For the first instance it will work properly and create a fresh connection, then for the second instance, it will not use the freshly created connection but close it and recreate a new one. Doing so, it breaks the connection of the first instance which will in turn force a new connection being open, breaking the one of the first instance, etc...., entering a loop.

My solution is to avoid sharing connections and create one for each instance even if they connect to the same host.

Hippolyte HENRY added 2 commits February 23, 2017 10:23
Change the key under which a connection is stored in the ConnectionManager to avoid shared connections
thus preventing entering a 'force new connection' loop.
@olivielpeau
Copy link
Member

Thanks @zippolyte for the fix!

I think the instanceName is not guaranteed to be unique across instances if 2 instances with the same host/port are defined in the same configuration file for example.

Also, even if the instanceName is unique, creating one connection per configured instance kind of defeats the purpose of the ConnectionManager class:

Singleton used to share connections across instances in case you have multiple instances configured to the same MBeanServer (e.g. Solr and Tomcat)

(see https://github.com/DataDog/jmxfetch/blob/0.12.0/src/main/java/org/datadog/jmxfetch/ConnectionManager.java#L11)

Instead of this solution, have you explored if we could keep sharing connections between instances and fix the logic that handles the connection refresh (so that it only refreshes the shared connection once and makes all the instances that use this shared connection use it)?

I'm not sure such a fix would be easy to implement, so if it doesn't look worth it we may want to keep your current solution. In that case we could improve this fix by ensuring that the instanceName is unique, and we should probably update the ConnectionManager class so that its only responsibility is creating a connection.

@zippolyte zippolyte changed the title Store connection under the key {instanceName} in ConnectionManager Fix JMXFetch issue when several instances on the same host Feb 24, 2017
@zippolyte
Copy link
Contributor Author

After some thinking, keeping the shared connections did not seem worth the trouble. Indeed it would imply making a lot more calls to the ConnectionManager to make sure the last version of the connection is used (it might have been refreshed by another instance) and passing this up-to-date connection to a lot of different objects methods (e.g. JMXAttribute.getValue). And the performance gained with these shared connections would be negligible since there are usually only two instances sharing a connection.
So it was decided that each instance would keep its own connection and the ConnectionManager would become a ConnectionFactory with the only purpose of creating connections.

@yannmh yannmh self-requested a review February 27, 2017 19:58
Copy link
Member

@yannmh yannmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
Let's merge it :shipit:

@zippolyte zippolyte merged commit 2ce3627 into master Feb 27, 2017
@zippolyte zippolyte deleted the hippolyte.henry/jmxfetch_multiple_connection branch February 27, 2017 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants